Skip to content

fix(tools): resolve hermes CLI beside the interpreter in bot_mode_dm deliveries - #100673

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-100662
Closed

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-100662

Conversation

@liuhao1024

Copy link
Copy Markdown

What does this PR do?

Bot-to-bot message_agent delivery builds both transport argvs — the local-teammate hermes -p <bot> chat … turn and the peer hermes -p <default> peer dm … transport — with a bare "hermes" as argv[0]. Since #96631 the delivery runner spawns via terminal_tool(..., _host_local=True), which uses an isolated host-local environment cache that does not inherit the gateway's PATH. On docker/service installs (venv at /opt/hermes/.venv, no hermes on the runner's minimal PATH) every delivery therefore acks status: queued and then the background runner exits 1 with FileNotFoundError: [Errno 2] No such file or directory: 'hermes' (#100662).

This PR resolves the CLI at both argv construction sites in tools/bot_mode_dm.py using bot_relay._hermes_cli() — the exact resolver the relay's deliver RPC adopted for the same class of failure in #93590 (venv sibling of this gateway's interpreter → shutil.which → bare-name fallback). The runner is spawned with the gateway's own sys.executable, so the sibling lookup is correct in the runner's context too. The turn-lock matcher in _delivery_lock() already matches argv[0] by basename (see its comment referencing #93590), so resolved absolute paths — and hermes.exe on Windows — take the per-profile lock exactly as before.

Related Issue

Fixes #100662

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/bot_mode_dm.py: resolve the delivery CLI via bot_relay._hermes_cli() in message_agent_tool and use it for both argv construction sites (peer dm and local teammate chat) instead of the bare "hermes".
  • tests/tools/test_bot_mode_dm.py: existing transport-argv assertions now expect _hermes_cli() as argv[0] — they previously pinned the bare name, i.e. they encoded the buggy behavior; added test_delivery_resolves_venv_sibling_cli, which fakes a docker-style venv (no PATH hit, sibling entrypoint beside sys.executable) and asserts both transports carry the absolute sibling path.

How to Test

  1. python -m pytest tests/tools/test_bot_mode_dm.py tests/tools/test_bot_relay_windows_paths.py tests/tools/test_bot_turn_lock.py tests/tools/test_bot_retry_policy.py tests/tools/test_bot_dm_payload_cache.py tests/tools/test_bot_mode_probe.py tests/tools/test_bot_relay.py tests/hermes_cli/test_chat_query_file.py -q → Observed result: 148 passed, 1 skipped (includes the new regression test).
  2. Simulated the reporter's docker shape without docker: monkeypatched sys.executable to a fake venv with a hermes sibling and no PATH resolution — both the local and peer transports now embed the absolute sibling path (new test), where before this PR they embedded bare hermes and died with ENOENT under the isolated runner env.
  3. Fallback behavior is unchanged and covered by the existing test_bot_relay_windows_paths.py suite (sibling → shutil.which → bare name).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (arm64), Python 3.11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A (Windows covered: resolver checks hermes.exe, lock matcher splits on both separators)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management backend/docker Docker container execution sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 1, 2026
@liuhao1024

Copy link
Copy Markdown
Author

The sole failing check here is Desktop E2E / Playwright E2E (Linux) (the "All required checks pass" aggregate fails for the same root cause). This looks like a pre-existing flake on main, not something introduced by this PR:

  • Failing test: e2e/correction-session-switch.spec.ts:188:3 › correction session switch › keeps a live correction in place and does not duplicate its original prompt after switching sessions
  • Failure mechanism: the CORRECTION transcript entry ("E2E correction must stay after the original prompt.") never rendered within the 30s waitForPredicate window, so the observed array is [ORIGINAL_PROMPT, CORRECTED_REPLY] instead of [ORIGINAL_PROMPT, CORRECTION, CORRECTED_REPLY] — a pure renderer-timing flake (it also failed Retry Terminal tool #1 the same way).
  • Clean main control: main @ d4611ac8 failed the identical test in the same time window — run 33554915907, job started 2026-09-01T20:26:03Z, and run 33554738281 (20:20Z) failed the same Playwright job as well.
  • Changed-file scope: this PR only touches tools/bot_mode_dm.py and tests/tools/test_bot_mode_dm.py (Python-side); it does not touch apps/desktop/ or any correction/session-switch code path, and all Python test/lint jobs are green.

Given the same fingerprint reproduces on clean main concurrently, I'm treating this as unrelated CI flake rather than a regression from this change.

@ryangu00

ryangu00 commented Sep 6, 2026

Copy link
Copy Markdown

Independent confirmation of #100662 on a different install shape, plus one extra failure mode this PR also covers:

  • macOS, dev-machine PATH (not PATH-less): the runner's sanitized env still had a PATH, but the first hermes on it was a wrapper for an older tree. The recipient CLI started, rejected --in ~ --create-if-missing --query-file (exit 2), and the DM was lost while message_agent had already returned status: sent. So the symptom is not only FileNotFoundError — any wrong hermes on PATH fails the same way, and [Bug]: notify_on_complete is dropped when the owning session's poller is not live, so Bot Mode handoff replies are destroyed #90879's one-shot linger sees nothing pending because the runner is already gone. Resolving the interpreter sibling (what this PR does) fixes both.
  • Root-caused it the same way you did: tools/bot_mode_dm.py still built both transport argvs with a bare "hermes" after fix(bot-relay): delivery works on Windows paths and PATH-less gateways (#93590, salvage #93601, credit #93597) #93658 fixed bot_relay.local_delivery_command.
  • Verified on main 245e480 with an equivalent local patch: the one-shot sender now logs One-shot exit lingering (bounded 600.0s) … completed=[…] and the recipient's Bot Chat receives the message (delivery ≈ 20 s end-to-end); scripts/run_tests.sh tests/tools/test_bot_mode_dm.py tests/tools/test_bot_relay_windows_paths.py tests/tools/test_oneshot_completion_linger.py → 69 passed, 1 skipped.

One suggestion: if the new test only pins the local-teammate transport, a symmetric assertion for the peer-dm argv ([-p <default> peer dm <peer>]) catches a regression of the second call site — happy to push that as a follow-up or as a suggestion on this PR if useful. Not opening a separate PR to avoid a duplicate.

@liuhao1024

Copy link
Copy Markdown
Author

Thanks for the independent confirmation and the extra failure mode — good to know the wrong-hermes-on-PATH shape (exit 2 rather than ENOENT) hits the same call sites, and that the interpreter-sibling resolution covers both.

On the test suggestion: the symmetric peer-dm assertion is already covered in this PR. test_peer_delivery_command_pins_registry_profile_for_secondary_bots and test_peer_delivery_command pin the full peer-dm argv element-by-element as [_hermes_cli(), "-p", "default", "peer", "dm", <peer>], and the new test_delivery_resolves_venv_sibling_cli exercises both transports in one test, asserting the resolved sibling path as argv[0] plus the ["-p", "default"] prefix on the peer-dm call. So the second call site is already regression-pinned — no follow-up needed.

…deliveries

Bot-to-bot message_agent delivery builds both transport argvs (local
teammate chat and peer dm) with a bare "hermes" as argv[0]. Since NousResearch#96631
the delivery runner spawns under terminal_tool's isolated host-local
environment, which does not inherit the gateway's PATH — so on
docker/service installs (venv at /opt/hermes/.venv) every delivery exits
with FileNotFoundError: 'hermes'.

Resolve the CLI with bot_relay._hermes_cli() (NousResearch#93590) — the venv sibling
of this interpreter, then shutil.which, then the bare name — at both
argv construction sites. The turn-lock matcher in _delivery_lock()
already matches argv[0] by basename, so absolute paths lock exactly as
before.

Fixes NousResearch#100662
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @liuhao1024. Your fix was carried with contributor history into #111298, now merged at b8bf484. We independently reproduced the failure and verified the repair in native Electron with real Hermes backends. Closing this original as landed through the salvage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/docker Docker container execution comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

4 participants